Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Refactor: Transaction #224

Merged
merged 6 commits into from
Aug 31, 2020
Merged

API Refactor: Transaction #224

merged 6 commits into from
Aug 31, 2020

Conversation

0xGabi
Copy link
Contributor

@0xGabi 0xGabi commented Aug 20, 2020

Closes #220

Builds on #223

@0xGabi 0xGabi requested a review from bpierre August 20, 2020 20:55
@0xGabi 0xGabi added this to In progress in Roadmap to v1.0.0 via automation Aug 20, 2020
@@ -59,6 +61,15 @@ export default class App {
return this.organization.connection.orgConnector
}

interface(): ethers.utils.Interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A helper to access an Ethers interface object from the app entities.

if (!app) {
throw new Error(`Could not create transaction due to missing app artifact`)
}
const appInterface = app.interface()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We refactor the logic to use Ethers interface directly instead of having the old logic to search for the methodAbiFragment manually.

): Promise<Transaction> {
let transactionOptions = {}

// If an extra parameter has been provided, it is the transaction options if it is an object
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't longer need to support the transaction options as overrides as we did on aragonAPI. Now, this is the responsibility of the users of Connect when they sign the transaction we provide.

spender: forwarderAddress, // since it's a forwarding transaction, always show the real spender
value: feeDetails.amount.toString(),
},
const tokenData: TokenData = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer pass the Token details as part of the transaction object instead we provide a new object.

@@ -1,76 +1,76 @@
import { ethers } from 'ethers'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent errors with types I decided to comment the logic related to the Forwarding Path descriptions.

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #224 into update-types will increase coverage by 1.24%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           update-types     #224      +/-   ##
================================================
+ Coverage         34.72%   35.96%   +1.24%     
================================================
  Files                99       97       -2     
  Lines              1823     1760      -63     
  Branches            291      277      -14     
================================================
  Hits                633      633              
+ Misses             1190     1127      -63     
Flag Coverage Δ
#unittests 35.96% <0.00%> (+1.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ages/connect-core/src/connections/ConnectorJson.ts 0.00% <ø> (ø)
packages/connect-core/src/entities/App.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Organization.ts 0.00% <ø> (ø)
packages/connect-core/src/entities/Transaction.ts 0.00% <0.00%> (ø)
packages/connect-core/src/index.ts 0.00% <ø> (ø)
...connect-core/src/transactions/TransactionIntent.ts 0.00% <ø> (ø)
...s/connect-core/src/transactions/TransactionPath.ts 0.00% <ø> (ø)
packages/connect-core/src/utils/app.ts 0.00% <0.00%> (ø)
packages/connect-core/src/utils/callScript.ts 0.00% <ø> (ø)
packages/connect-core/src/utils/index.ts 0.00% <ø> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e36825a...068a952. Read the comment docs.

@0xGabi 0xGabi moved this from In progress to In review in Roadmap to v1.0.0 Aug 21, 2020
@0xGabi 0xGabi merged commit f216a95 into update-types Aug 31, 2020
Roadmap to v1.0.0 automation moved this from In review to Done Aug 31, 2020
@0xGabi 0xGabi deleted the refactor-transaction branch August 31, 2020 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

1 participant